Repository navigation
Conversation
… returns A callback that takes `done` ends through `done()`. The runner no longer calls `then()` on an object that such a callback also returns. Since #44833 the runner followed a thenable that a `done` callback returned. That sent a supertest request twice: `done => request(app).get("/").expect(200, done)` failed with ".end() was called twice". Bun 1.4.4, Jest 30.5.2 and `node --test` 26.3.0 do not call `then()` there.
 |
| Layer / File(s) | Summary |
|---|---|
Done callback behavior src/runtime/test_runner/bun_test.rs, test/js/bun/test/vitest-flavor.test.ts, docs/test/lifecycle.mdx |
Callbacks with a done parameter use the callback directly instead of converting its return value to a promise. Tests cover hooks, ordinary tests, test.each, rejection, non-settlement, timeout, and done(error). The lifecycle documentation states that Bun does not call then() on the returned object. |
Suggested reviewers: jarred-sumner, alii
Merge Risk: 
fa960
-
The done-callback change has no established runtime regression. The two explicit test timeouts need a small cleanup or a justified exception; this is a bounded maintenance concern.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: Bun no longer calls then() on values returned by callbacks that accept done. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The description follows the repository template with both required sections present: "What does this PR do?" is covered by the Problem, Fix, and Background subsections, and "How did you verify your co… Full details: Description check
-
Explanation
The description follows the repository template with both required sections present: "What does this PR do?" is covered by the Problem, Fix, and Background subsections, and "How did you verify your code works?" is covered by the Verified line, the Tests section, and the Measurements section. It explains the cause (run_test_callback passes every returned value to promise_of_thenable without checking cfg_done_parameter), the behavior change for done callbacks, the tradeoff (a rejecting or never-settling thenable returned from a done test now passes, as on 1.4.4), and the new regression tests in test/js/bun/test/vitest-flavor.test.ts, which fail on canary and pass with the fix. The docs change in docs/test/lifecycle.mdx is also mentioned. The description is detailed and specific. The template headings are not reproduced verbatim, but the content maps onto both required sections.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
CodeRabbit Verify
- Run CodeRabbit Verify to test the changes in this pull request.
|
Reproduced on the canary of main,
The new test in CI (build 137060): the red files are PR: #47730 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline nit, I also checked what happens when a done callback returns a callable thenable and calls done() synchronously: the raw value now reaches adopt_teardown, but that function returns early unless calling.is_vitest(), and a done parameter only exists for Calling::Jest, so no teardown is registered. The dropped rejection of a returned thenable under done is the intended trade-off the description states, not a defect in the gate.
Extended reasoning...
The change gates the promise_of_thenable step in BunTest::run_test_callback on cfg_done_parameter in src/runtime/test_runner/bun_test.rs, adds one doc sentence to docs/test/lifecycle.mdx, and adds a spawned-fixture test matrix for bun:test and @ jest/globals in test/js/bun/test/vitest-flavor.test.ts; it touches no security-sensitive surface. One inline nit about a 10 ms timeout in the fixture was posted; the two candidate correctness issues were traced in the code and do not apply.
There was a problem hiding this comment.
I reviewed the updated PR and didn't find any bugs; the follow-up commit addressed the timeout concern from my earlier inline comment. A human look is still worthwhile only for the one policy choice this makes: a thenable returned by a done callback is now ignored (its rejection is never seen), which restores 1.4.4 behaviour rather than failing the test the way Jest does.
What was reviewed:
run_test_callbackin src/runtime/test_runner/bun_test.rs:cfg_done_parameterisCalling::Jest { done: true }from Execution.rs, so vitest callbacks (where the parameter is the context) are unaffected, and native promises returned withdonestill go through the existingas_promise()state tracking.- The fixture's
logassertion (exactly threesentlines, nothen()lines) is the load-bearing check that nothen()is called from a hook, a test, ortest.each; the 10 ms timeout is only on the case expected to fail, and the timeout message is indented so it does not leak intoerrors. - Doc sentence in docs/test/lifecycle.mdx matches the implemented behaviour.
Extended reasoning...
The change is a one-line conditional in src/runtime/test_runner/bun_test.rs that skips promise_of_thenable when the callback declares a done parameter, plus a subprocess test in test/js/bun/test/vitest-flavor.test.ts covering bun:test and @ jest/globals and a one-sentence doc update. It touches no security-sensitive surface. The code is small and the test exercises hooks, tests, test.each, rejecting and never-settling thenables, and both done() outcomes, and the author's latest commit resolved the only prior inline nit. Defer rather than approve because the PR deliberately chooses to silently ignore a returned thenable's rejection in done tests (1.4.4 behaviour) instead of erroring like Jest, a user-facing test-runner semantics decision a maintainer should confirm; no CODEOWNERS entry covers these paths and no third-party objections are outstanding.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…-callback-no-then
Problem
bun testcallsthen()on a thenable that a test or a hook returns, also when the callback takesdone. supertest'sdone => request(app).get("/").expect(200, done)then fails:.end() was called twice. 1.4.4 passes it.run_test_callback(src/runtime/test_runner/bun_test.rs:1392) gives every returned value topromise_of_thenable. It does not readcfg_done_parameter.Fix
doneparameter the runner skipspromise_of_thenable: nothen()call on the returned value, as on 1.4.4. A returned promise is still awaited by its state.done(). Jest 30.5.2 andnode --test26.3.0 call nothen()there.test/js/bun/test/vitest-flavor.test.ts(2 new tests, both fail on canary3f3a9ff87). Alsotest/js/bun/test/,node-test.test.ts. Self-reviewed: 4 concerns raised, 4 addressed.Background
then()that is not a promise. A supertest request is one:end()sends it, andthen()callsend().doneis the parameter of a callback frombun:testor@jest/globals. Withvitestit is the test context, and nothing changes.Test functions cannot both take a 'done' callback and return something). That fails suites that pass on 1.4.4.Downsides
donetest whose returned thenable rejects or never settles fails on main. With this PR it passes, as on 1.4.4.sizetext unchanged.Notes
What changes
Each row was run on 1.4.4 (38afee6), on the canary of main (
1.4.4-canary.1+3f3a9ff87, with #44833) and on a release build of this branch. Every callback takesdone.done => request(app).get("/").expect(200, done)(supertest 6.3.3).end() was called twice. superagent warns that the request was sent twicedone => request(app).get("/").expect(200).end(done)expected 200 "OK", got 404 "Not Found".end() was called twiceexpected 200 "OK", got 404 "Not Found"done(); returna thenable that rejectsthen()callsdone(); returna thenable that never settlesdone()done(error)laterdone()done()and the thenable. This PR waits fordone()only. That is the cost of the rule: a rejection of a thenable that adonetest returns is not seen, as on 1.4.4.donedoes not change. It still waits on a returned thenable (Bun test runner passes supertest request, where Jest fails #27945).Other runners
Run with a returned thenable and a returned Promise subclass in a
donetest. Each makes 0then()calls.Test functions cannot both take a 'done' callback and return something. Either use a 'done' callback, or return a promise.node --test26.3.0 ignores the thenable and passes. It fails a returned promise:passed a callback but also returned a Promise.donecallback.Related
then()of a returned thenable before the microtask drain), bun:test: fixtures and around hooks wait on what await waits on, through the engine #47800 (fixtures and around hooks wait through the engine) and bun:test: wait on a Promise subclass through its own then() #41008 (a Promise subclass through its ownthen()) are on top of this PR, in that order. This PR can merge first, alone.donecase: a returned promise that is already fulfilled ends the test withoutdone().docs/test/lifecycle.mdxsaid that Bun waits for any returned object with athen()method. It now names thedoneexception.Measurements
Release builds of main (3f3a9ff) and of this branch from one build directory, linux-x64, JIT off.
run_test_callback, callees included, per call:undefined982 to 982, plain object 1380 to 1380, pending native promise 5350 to 5350,done => done()3242 to 3242. Method: a gdb single-step from the entry of the function to its return, at the fourth and the fifth call.sizetext: 140287599 to 140287599.run_test_callback2426 B to 2428 B.perfandvalgrindare not in the container.Tests
test.each, a thenable that rejects, one that never settles, adone()that is never called and a latedone(error). It passes on 1.4.4 and fails on main.vitest-flavor.test.ts,done-async.test.ts,test-test.test.ts,bun_test.test.ts,concurrent.test.ts,jest-hooks.test.ts,test-error-code-done-callback.test.tsandnode-test.test.ts: 306 pass. The new test is clean withBUN_JSC_validateExceptionChecks=1.test/js/bun/test/4188 pass, 1 fail. The failure is "error snapshots" insnapshot.test.ts, which expects a TTY and fails on the release build of main too.[auto-merge] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file